[ThemeProvider] Optimize theme changes when enabling CSS theme variables
#44588
Conversation
siriwatknp
added
performance
package: material-ui
Netlify deploy previewhttps://deploy-preview-44588--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
| it('theme should remain the same when ThemeProvider rerenders', () => { | ||
| const theme = createTheme({ cssVariables: true }); | ||
|
|
||
| function Inner() { | ||
| const upperTheme = useTheme(); | ||
| const [count, setCount] = React.useState(0); | ||
| React.useEffect(() => { | ||
| setCount((prev) => prev + 1); | ||
| }, [upperTheme]); | ||
| return <span data-testid="inner">{count}</span>; | ||
| } | ||
| function App() { | ||
| const [count, setCount] = React.useState(0); | ||
| return ( | ||
| <ThemeProvider theme={theme}> | ||
| <button onClick={() => setCount(count + 1)}>rerender</button> | ||
| <Inner /> | ||
| </ThemeProvider> | ||
| ); | ||
| } | ||
| render(<App />); | ||
|
|
||
| fireEvent.click(screen.getByRole('button')); | ||
|
|
||
| expect(screen.getByTestId('inner')).to.have.text('2'); // due to double rendering | ||
| }); |
There was a problem hiding this comment.
Should theme and upperTheme be referentially equal? I was about to suggest that we could use another approach for the test: instead of counting renders, check that theme === upperTheme, but I found that they're not referentially equal. Not sure if it's possible to do, just wondering if that's something we should aim for.
it('theme should remain the same when ThemeProvider rerenders', () => {
const theme = createTheme({ cssVariables: true });
function Inner() {
const upperTheme = useTheme();
return <span data-testid="inner">{(theme === upperTheme).toString()}</span>;
}
function App() {
return (
<ThemeProvider theme={theme}>
<Inner />
</ThemeProvider>
);
}
render(<App />);
expect(screen.getByTestId('inner')).to.have.text('true');
});There was a problem hiding this comment.
Good point but const theme = createTheme({ cssVariables: true }); is not equal to const upperTheme = useTheme(); because it's recalculated within ThemeProvider.
I could write a different test using React.useRef to compare the previous and the incoming theme so that it's not rely on count.
closes #44555
Context
ThemeProviderrecompose the theme and pass it to React context. The current implementation is not optimized, so if the parent ofThemeProviderrerenders, the theme send to React context is a new object causing the component that get the theme throughuseThemeto rerender (or run effect) again.